internal/exec/describe_affected_test.go (1)

2079-2168: Consider adding a test for OutputFile scenario.

The new conditional in uploadableQuery has three branches: !args.Upload, args.Verbose, and args.OutputFile != "". The tests cover the first two but not the third. A test verifying that output is shown when Upload=true, Verbose=false, and OutputFile is set would complete the coverage.

Example test case

// TestUploadShowsOutputWhenFileSet verifies that when --upload and --file are both set,
// the JSON output is written to file (printOrWriteToFile is called).
func TestUploadShowsOutputWhenFileSet(t *testing.T) {
	printCalled := false

	d := describeAffectedExec{
		atmosConfig: &schema.AtmosConfiguration{},
		printOrWriteToFile: func(atmosConfig *schema.AtmosConfiguration, format, file string, data any) error {
			printCalled = true
			return nil
		},
		IsTTYSupportForStdout: func() bool { return false },
		pageCreator:           pager.New(),
	}

	headRef := plumbing.NewHashReference("refs/heads/feature", plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"))
	baseRef := plumbing.NewHashReference("refs/heads/main", plumbing.NewHash("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"))

	affected := []schema.Affected{
		{Component: "vpc", Stack: "dev"},
	}

	err := d.uploadableQuery(
		&DescribeAffectedCmdArgs{
			Upload:      true,
			Verbose:     false,
			OutputFile:  "/tmp/affected.json",
			Format:      "json",
			CIEventType: "pull_request",
			CLIConfig:   &schema.AtmosConfiguration{},
		},
		"https://github.com/example/repo.git",
		headRef,
		baseRef,
		affected,
	)

	assert.True(t, printCalled, "printOrWriteToFile should be called when --upload and --file are both set")
	assert.NoError(t, err)
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/exec/describe_affected_test.go` around lines 2079 - 2168, The test
suite is missing coverage for the upload branch when OutputFile is provided; add
a new unit test that calls describeAffectedExec.uploadableQuery with
DescribeAffectedCmdArgs{Upload:true, Verbose:false,
OutputFile:"/tmp/affected.json", Format:"json", CIEventType:"pull_request",
CLIConfig:...} using a describeAffectedExec whose printOrWriteToFile stub flips
a boolean, then assert printOrWriteToFile was called and that uploadableQuery
returns no error; reference the same setup as
TestUploadSuppressesOutputButStillUploads/TestUploadShowsOutputWhenVerbose
(headRef, baseRef, affected, IsTTYSupportForStdout, pageCreator) to keep tests
consistent.

internal/exec/describe_affected_test.go (1)

2100-2186: Consolidate upload output mode tests into table-driven cases.

These two tests are the same harness with mode toggles (Verbose false/true). A table-driven test will reduce duplication and make future branches easier to add.

♻️ Proposed refactor sketch

+func TestUploadOutputModes(t *testing.T) {
+  cases := []struct {
+    name      string
+    verbose   bool
+    wantCalls int
+  }{
+    {name: "suppressed when upload without verbose", verbose: false, wantCalls: 0},
+    {name: "shown when upload with verbose", verbose: true, wantCalls: 1},
+  }
+
+  for _, tc := range cases {
+    t.Run(tc.name, func(t *testing.T) {
+      calls := 0
+      d := describeAffectedExec{
+        atmosConfig: &schema.AtmosConfiguration{},
+        printOrWriteToFile: func(*schema.AtmosConfiguration, string, string, any) error {
+          calls++
+          return nil
+        },
+        IsTTYSupportForStdout: func() bool { return false },
+        pageCreator:           pager.New(),
+      }
+      // ... invoke uploadableQuery with tc.verbose ...
+      assert.Equal(t, tc.wantCalls, calls)
+    })
+  }
+}

As per coding guidelines, "Use table-driven tests for testing multiple scenarios in Go."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/exec/describe_affected_test.go` around lines 2100 - 2186, Combine
the two near-identical tests TestUploadSuppressesOutputButStillUploads and
TestUploadShowsOutputWhenVerbose into a single table-driven test (e.g.,
TestUploadOutputModes) that iterates cases for Verbose=false and Verbose=true;
for each case create the describeAffectedExec with the same printOrWriteToFile
stub, IsTTYSupportForStdout, pageCreator, headRef/baseRef and affected slice,
call uploadableQuery with fields driven from the table row, and in a t.Run
subtest assert whether printCalled matches the expected value and that err is
nil; this reduces duplication and keeps the same assertions and setup while
using table rows to add future cases.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

internal/exec/describe_affected_test.go (1)

2728-2732: Conditional assertion may weaken test guarantees.

The if describe.SHA == "" branch makes this test pass for two different outcomes. For a pull_request event with the given payload, the expected behavior should be deterministic. If the provider can legitimately return either Ref or SHA, consider splitting into separate sub-tests or documenting why both outcomes are acceptable.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/exec/describe_affected_test.go` around lines 2728 - 2732, The test
currently allows two different outcomes by using "if describe.SHA == \"\"" which
weakens guarantees; change the test to be deterministic by either asserting
exactly the expected field (Ref or SHA) for the specific pull_request payload or
split into two explicit sub-tests that validate each allowed provider behavior
separately: locate the test that reads the describe variable (references to
describe.SHA and describe.Ref) and replace the conditional with one of two fixes
— (A) assert the single expected value (e.g., require.Equal on describe.SHA or
describe.Ref) for this payload, or (B) create two sub-tests
(t.Run(\"returns-sha\", ...) and t.Run(\"returns-ref\", ...)) each with a
fixture or mocked provider that deterministically returns SHA or Ref and assert
the corresponding field; ensure the assertions are precise (use require/expect)
and add a short comment explaining why both behaviors are tested if you choose
option B.